Skip to content

sta_config_equal #5937

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Apr 1, 2019
Merged

sta_config_equal #5937

merged 3 commits into from
Apr 1, 2019

Conversation

dontsovcmc
Copy link
Contributor

There is a bug with reconnect to the same WiFi after running Active Point.
Long description: tzapu/WiFiManager#782

sta_config_equal(conf_compare, conf) doesn't compare all fields.
https://github.com./esp8266/Arduino/blob/master/libraries/ESP8266WiFi/src/ESP8266WiFiSTA.cpp#L160

This is a reason of failed connection for me. Maybe you can test it for another cases.

Copy link
Collaborator

@d-a-v d-a-v left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@dontsovcmc
Copy link
Contributor Author

How we can prevent future bugs?
two independent commits broke it:
429f40d 19.02.2018
8ef21ca 09.10.2018

I see 2 ways:

  1. comment in user_interface.h "please, add fields to 'here' and 'here'..."
  2. inialize structures with 0. This way we can use memcmp function and be independent from fields.

@d-a-v
Copy link
Collaborator

d-a-v commented Apr 1, 2019

How we can prevent future bugs?

I'm not sure.

Maybe by adding a comment in user_interface.h(edit: like you said) and/or by constexpr-checking sizeof(the-structs-used-for-comparison) against a constant (that has to be changed when more fields appear) (static_assert())

@tablatronix
Copy link
Contributor

Thanks @dontsovcmc I did not notice these updates, good catch, and yes this has been an issue in the past also when this struct changes.

I do like the struct integrity checking using a size constraint assert.

@devyte devyte merged commit 68c0a1c into esp8266:master Apr 1, 2019
@d-a-v
Copy link
Collaborator

d-a-v commented Apr 1, 2019

I forgot to mention #5939 (keeping track of reasons)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants